Skip to content

Parallelize #1960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Parallelize #1960

wants to merge 13 commits into from

Conversation

kderme
Copy link
Contributor

@kderme kderme commented Mar 21, 2025

Description

TODO:

  • the generated tx.id are generated as block.id * 100000 + tx_index. The 100000 is chosen to avoid duplications, since the genesis virtual block has ~14000 txs, . A cache could be used to get the last id for each block.
  • Commiting the main block transaction should first wait for all dependencies transactions to be commited
  • Check db to avoid reinstering rewards if there is a restart.
  • soptEpochAndCacheEnabled is hardcoded False. Fix it.
  • forcing the rewards from the ledger state needs major simplification and may be bugged (see genericRewardUpdate). Currently the ledger functions that compute them live in ReaderT Global even though the Globals are never used. This could be improved in the ledger api, by providing something similar to forceDRepPulsingState
  • Remove reduntant Promise.hs and TxInResolve.hs
  • Threads should not commit if nothing happened since the last commit. Otherwise warning are raised.

Further improvements:

  • In function applyAndInsertBlocks we split the blocks based on byron or not. We should also split them for epochboundaries. To achieve this, we need to predict the slot details of the whole list of blocks, before the ledger application is done. This is possible since we can predict the slot details up to a specific horizon
  • Use the Prepared tx type, so that all preparations happen in parallel.

Checklist

  • Commit sequence broadly makes sense
  • Commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the changelog
  • Code is formatted with fourmolu on version 0.10.1.0 (which can be run with scripts/fourmolize.sh)
  • Self-reviewed the diff

Migrations

  • The pr causes a breaking change of type a,b or c
  • If there is a breaking change, the pr includes a database migration and/or a fix process for old values, so that upgrade is possible
  • Resyncing and running the migrations provided will result in the same database semantically

If there is a breaking change, especially a big one, please add a justification here. Please elaborate
more what the migration achieves, what it cannot achieve or why a migration is not possible.

@kderme kderme force-pushed the kderme/parallelize branch from c024537 to 064aaf1 Compare March 24, 2025 10:43
@kderme kderme force-pushed the kderme/parallelize branch from ff015c7 to a6f18a4 Compare April 24, 2025 12:41
@kderme kderme force-pushed the kderme/parallelize branch from a6f18a4 to 7209d87 Compare April 25, 2025 10:07
@kderme kderme force-pushed the kderme/parallelize branch from 92c9850 to 3373fee Compare April 30, 2025 20:56
@kderme kderme force-pushed the kderme/parallelize branch from 898c1d1 to b4934be Compare April 30, 2025 22:15
Copy link
Contributor

@Cmdv Cmdv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking very good so far, just very minor comments

@@ -237,6 +243,7 @@ extractSyncOptions snp aop snc =
not isTxOutConsumedBootstrap'
&& ioInOut iopts
&& not (enpEpochDisabled snp || not (enpHasCache snp))
&& False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is for testing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I have it in the TODO list of the pr

@@ -390,10 +346,59 @@ mkSyncEnv trce backend connectionString syncOptions protoInfo nw nwMagic systemS
where
hasLedger' = hasLedger . sioLedger . dncInsertOptions
isTxOutConsumedBootstrap' = isTxOutConsumedBootstrap . sioTxOut . dncInsertOptions
backend = mainBackend backends

withDBSyncConnections ::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these maybe not be inside of Cardano.Db.Run

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it could be. I thought DbConnections as something that DBSync maintains and could change based on its needs, but this is debatable

@@ -100,3 +114,22 @@ data ConsistentLevel = Consistent | DBAheadOfLedger | Unchecked
newtype CurrentEpochNo = CurrentEpochNo
{ cenEpochNo :: Strict.Maybe EpochNo
}

data DbConnections = DbConnections
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have conflicts with these as hasql doesn't use these types :)

import Cardano.DbSync.Cache.Epoch (rollbackMapEpochInCache)
import qualified Cardano.DbSync.Cache.FIFO as FIFO
import qualified Cardano.DbSync.Cache.LRU as LRU
import Cardano.DbSync.Cache.Types (CacheAction (..), CacheInternal (..), CacheStatistics (..), CacheStatus (..), StakeCache (..), initCacheStatistics, shouldCache)
import Cardano.DbSync.Cache.Stake as X
import Cardano.DbSync.Cache.Types
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also expose these modules same a Cardano.DbSync.Cache.Stake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stake is the only part of Cache which is splitted in its own module.
If you're refering to Cardano.DbSync.Cache.Types, we don't usually reexpose the Types through its most related module. eg see Cardano.DbSync.Api.(Types)

import qualified Data.Map.Strict as Map
import Database.Persist.Postgresql (SqlBackend)

-- | TO be called only by the stake thread
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you fancied it you could have a type level constraint on thread specific functions:

-- Define thread-specific phantom types
data StakeThread
data BlockThread
data TxThread

-- Create a type that represents actions constrained to specific threads
newtype ThreadConstrained thread m a = ThreadConstrained { runThreadConstrained :: m a }
  deriving (Functor, Applicative, Monad, MonadIO, MonadBaseControl IO)

-- Modify your function to be constrained to the stake thread
resolveInsertRewardAccount ::
  forall m.
  (MonadBaseControl IO m, MonadIO m) =>
  SyncEnv ->
  CacheAction ->
  RewAccount ->
  ThreadConstrained StakeThread (ReaderT SqlBackend m) DB.StakeAddressId
resolveInsertRewardAccount syncEnv cacheUA ra = ThreadConstrained $ do
  eiStakeId <- queryStakeAddrWithCacheRetBs syncEnv cacheUA False ra -- read only
  case eiStakeId of
    Right stakeId -> pure stakeId
    Left (_, bs) -> insertStakeAddress ra (Just bs)

-- Then in your stake thread code, you would run it like:
runStakeThread :: MonadIO m => ReaderT SqlBackend m ()
runStakeThread = do
  -- Get your parameters from somewhere
  let syncEnv = ...
      cacheUA = ...
      ra = ...
  -- Run the thread-constrained function
  stakeId <- runThreadConstrained (resolveInsertRewardAccount syncEnv cacheUA ra)
  -- Do something with stakeId
  ...

-- If someone tries to call it from a different thread, they'd need to write:
runBlockThread :: MonadIO m => ReaderT SqlBackend m ()
runBlockThread = do
  -- This won't type-check because BlockThread ≠ StakeThread
  stakeId <- runThreadConstrained (resolveInsertRewardAccount syncEnv cacheUA ra)
  ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be yes. Once this is settled, it can become more type safe.

where
txId = DB.TxKey $ fromIntegral txIndex + 20000 * DB.unBlockKey blkId -- On mainnet genesis produces ~14000 txs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a little comment as to why we're not using the returned txId from running insertTx. I can't fully remember why :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personal note to add

We're using app generated unique ids to avoid dependencies between tables, which can damage 
parallelism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants